-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
types: per comment, avoid copy when t is not bound #32176
Conversation
0c52cc8
to
bccd664
Compare
@@ -1287,7 +1306,10 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value | |||
} | |||
else if (cacheable) { | |||
// recursively instantiate the types of the fields | |||
ndt->types = inst_ftypes(ftypes, env, stack); | |||
if (dt->types == NULL) | |||
ndt->types = jl_compute_fieldtypes(ndt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's reveled from an interaction of the PR with lazy-ftypes (although just the type-stack alone could theoretically run into the same problem with this PR). Previously, on arriving in this function, the dt was always the wrapper object and so the env was already that which will be created by jl_compute_fieldtypes
. With the PR, it now could be a partially computed type that just needs to be copied, but that also means it could now be the final parameter application to create a concrete type (where with lazy-ftypes, we are now not been computing the ftypes of that intermediate work).
As mentioned in #9378, be more careful about not copying objects that are already cached. This should get us the same result, but do strictly less work. And it should address the recursion issue mentioned in #25796, by avoiding making copies of any non-bound objects (which is anything maybe-cacheable) now. It's harder to trigger this now that #31877 is merged, but I think still worthwhile to do the cleanup (and to rely less on that optimization).